[CODE HEALTH] Fix clang-tidy bugprone-exception-escape warnings in API#3964
[CODE HEALTH] Fix clang-tidy bugprone-exception-escape warnings in API#3964dbarker wants to merge 9 commits intoopen-telemetry:mainfrom
Conversation
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #3964 +/- ##
==========================================
- Coverage 90.13% 90.05% -0.08%
==========================================
Files 230 230
Lines 7293 7304 +11
==========================================
+ Hits 6573 6577 +4
- Misses 720 727 +7
🚀 New features to boost your workflow:
|
…scape warning in string_util, Address review feedback.
…string utils test.
marcalff
left a comment
marcalff
left a comment
There was a problem hiding this comment.
The change to replace nostd::holds_alternative with nostd::get_if is clean and a no brainer.
Please prepare a separate patch to fix all nostd::holds_alternative so this part can be merged out of the way.
This PR can then focus on the try/catch code alone, as it will be more tricky to resolve.
| # define OPENTELEMETRY_UNREACHABLE() __assume(0) | ||
| #else | ||
| # include <cstdlib> | ||
| # define OPENTELEMETRY_UNREACHABLE() abort() |
There was a problem hiding this comment.
Should the fallback abort or something else?
| if (pos > length_) | ||
| { | ||
| # if __EXCEPTIONS | ||
| # if OPENTELEMETRY_HAVE_EXCEPTIONS |
There was a problem hiding this comment.
The behavior seems to have been inconsistent in the past between compilers (msvc is not setting __EXCEPTIONS). Changing to use the common macro seems reasonable and expected here, but is a change of behavior on msvc (which would now compile with this throw for string_view unless exceptions are disabled).
Thanks @marcalff. I've split this up. The nostd::variant access changes are now in #3965. Interested in feedback on the try/catch/unreachable macros given our need to support noexcept builds and analyzers. |
| { | ||
| return str.substr(left, 1 + right - left); | ||
| } | ||
| OPENTELEMETRY_CATCH_ALL |
There was a problem hiding this comment.
Once we start to add these to the sdk we may want to expand these macros to support passing information to the logger (exception message and other context from the call site).
| OPENTELEMETRY_TRY | ||
| { | ||
| return TraceState::GetDefault(); | ||
| if (!IsValidKey(key)) |
There was a problem hiding this comment.
One alternative to consider is to make IsValidKey/IsValidValue noexcept and put the try catch in those methods. That is an API impact but perhaps not an ABI change. The std::regex ctor and std::regex_match may throw.
2 participants
Contributes to #2053
Changes
bugprone-exception-escape (4 warnings)
opentelemetry-cpp/api/include/opentelemetry/trace/trace_state.hopentelemetry-cpp/api/include/opentelemetry/trace/trace_state.hopentelemetry-cpp/api/include/opentelemetry/trace/trace_state.hopentelemetry-cpp/api/include/opentelemetry/trace/trace_state.hFor significant contributions please make sure you have completed the following items:
CHANGELOG.mdupdated for non-trivial changes